WCN-604

Review

Overview

Adds a prepack script to modules/bitgo that generates npm-shrinkwrap.json in an isolated temp copy (stripping devDependencies/scripts, injecting root overrides, resolving against the real registry), ships it via the files array, and adds a post-publish workflow step that fails the release if npm's dist.hasShrinkwrap isn't true. The mechanism is sound — a dependency's npm-shrinkwrap.json is honored by npm to pin that dependency's own subtree, so this is the right lever for the goal.

🔴 Critical — sibling version resolution at pack time

modules/bitgo depends on 88 @bitgo/* siblings, and the shrinkwrap is generated by running npm install --package-lock-only against the public npm registry. prepack fires inside yarn lerna publish, which bumps versions and packs. At the moment bitgo is packed, the newly-bumped sibling versions are almost certainly not yet published/propagated on registry.npmjs.org. Two failure modes, both bad:

1. npm install can't resolve @bitgo/sdk-core@<new-version> → pack fails → every release blocked.
2. It resolves the previous published versions → the shipped shrinkwrap pins stale sibling versions, arguably worse than no shrinkwrap.

The canary plan won't surface this — a standalone bitgo@0.0.0-canary.N publishes against an already-published sibling tree, hiding the exact ordering hazard of a coordinated monorepo release. Recommend validating against a real/dry-run lerna release, and consider resolving siblings from the local workspace tarballs rather than the registry.

🟠 Significant

• prepack now requires network + a full npm install on every pack, including local/dev/offline packing (slow 88+ dep resolution; hard failure offline). Consider gating on a CI/release env var so local packs aren't broken.
• hasShrinkwrap verification may permanently false-fail. The PR body admits dist.hasShrinkwrap was never confirmed on a real npm publish; if npm doesn't populate it, this step fails every release. Querying the registry immediately post-publish is also subject to metadata propagation lag — add retry/backoff (the workflow already uses that pattern elsewhere). Confirm the field is real before merge.
• Undeclared dependency on root-hoisted tsx/execa. Both are root devDependencies, not declared in modules/bitgo. Fine within the hoisted workspace publish, but the script silently relies on hoisting.

🟡 Minor / style

• const { devDependencies, scripts, ...isolatedPackageJson } — confirm eslint no-unused-vars has ignoreRestSiblings: true, else this lint-fails.
• Script otherwise matches existing scripts/*.ts conventions (execa default import, main().catch(...), process.exit(1)). 👍
• if (!rootPackageJson.overrides) fail-fast is good; correctly depends on fix(root): add npm overrides mirroring yarn resolutions #9212.
• Consider logging which versions got pinned for release auditability, given the stale-pin risk.

Test coverage

No automated test (reasonable for release tooling), but given the sibling-timing risk, the unchecked canary/dry-run boxes should be treated as merge blockers.

Bottom line

Right mechanism, but the pack-time sibling resolution against the public registry is a real correctness/reliability hazard for a coordinated monorepo release, and the hasShrinkwrap gate could block releases if that metadata field isn't actually populated. Validate both on a real (dry-run) release before merge — the standalone canary alone is insufficient.

Generated by Claude Code

Thanks for the review — the critical finding is correct and I hadn't accounted for the sibling-publish race. Fixed:

• Critical (sibling resolution race): @bitgo/* deps are now excluded from the temp package.json before resolution, so npm install never touches the public registry for a just-bumped-but-not-yet-published sibling. Consumers resolve @bitgo/* siblings normally at install time (by then they're published); everything transitively pulled in beneath a sibling still gets pinned via overrides. The shrinkwrap's own dependencies field is patched back to the real sibling list after generation so what's shipped still matches package.json.
• prepack forcing network installs on local/offline packs: generation is now gated behind BITGO_GENERATE_SHRINKWRAP=true, set only on the release workflow's publish steps.
• hasShrinkwrap propagation lag: wrapped the registry check in the job's existing retry binary (--up-to 5x --every 5s) instead of a single curl.
• Lint: dropped the unused-var destructure (no ignoreRestSiblings in this repo's eslint config) in favor of explicit delete.
• Undeclared tsx/execa — left as-is, intentionally relying on root hoisting like other scripts/*.ts in this repo call out.

Agreed the canary-only validation was insufficient for the sibling-timing hazard specifically — that risk is now structurally eliminated rather than something to catch in testing. Canary/dry-run validation of the hasShrinkwrap metadata assumption is still outstanding before merge.

Claude finished @mihir-bitgo's task —— View job

Code Review Complete

• Read and analyze the changed files
• Review the generate-bitgo-shrinkwrap.ts script implementation
• Assess workflow changes and package.json modifications
• Evaluate if Otto's concerns about sibling resolution race have been addressed
• Provide comprehensive code review feedback

Review Summary

This PR effectively addresses the critical sibling resolution race condition identified by Otto while implementing a sound mechanism for shipping npm-shrinkwrap.json to npm consumers.

✅ Key Strengths

Sibling Resolution Race Fix

The implementation correctly addresses Otto's critical concern by:

• Excluding @bitgo/* siblings from temp package.json before resolution (scripts/generate-bitgo-shrinkwrap.ts:59-61)
• Restoring real dependencies in the final shrinkwrap (scripts/generate-bitgo-shrinkwrap.ts:78-81)
• This prevents resolution failures or stale version pinning during coordinated monorepo releases

Environment Gating

• BITGO_GENERATE_SHRINKWRAP=true guard (scripts/generate-bitgo-shrinkwrap.ts:34-37) prevents local/offline pack operations from requiring network access
• Only runs during CI release workflow (npmjs-release.yml:296)

Registry Verification

• Retry-wrapped hasShrinkwrap check (npmjs-release.yml:344-351) handles metadata propagation lag
• Fails release if npm doesn't set dist.hasShrinkwrap: true, ensuring consumers actually get the shrinkwrap

Clean Implementation

• Proper temp directory cleanup (scripts/generate-bitgo-shrinkwrap.ts:86)
• Explicit error handling with process.exit(1) (scripts/generate-bitgo-shrinkwrap.ts:90-93)
• Dependency injection of root overrides (scripts/generate-bitgo-shrinkwrap.ts:62)

🟡 Minor Observations

Code Style

• Direct property deletion (scripts/generate-bitgo-shrinkwrap.ts:57-58) avoids unused variable lint issues - good choice given this repo's eslint config
• Consistent logging with existing scripts/*.ts patterns

Dependencies

• Implicit dependency on root-hoisted tsx/execa - acceptable per repo conventions, though worth noting for future maintainers

Package.json Changes

• files array inclusion (modules/bitgo/package.json:186) correctly ships the generated shrinkwrap
• prepack script timing is optimal - runs after version bump but before tarball creation

🔍 Edge Cases Handled

1. Missing overrides block - Fails fast with clear error (scripts/generate-bitgo-shrinkwrap.ts:42-44)
2. Failed shrinkwrap generation - Validates file existence (scripts/generate-bitgo-shrinkwrap.ts:70-72)
3. Registry propagation lag - Retries metadata check up to 5x (npmjs-release.yml:344)
4. Package structure variations - Handles both flat and nested dependency structures (scripts/generate-bitgo-shrinkwrap.ts:79-81)

Recommendation

✅ Approve - The implementation correctly solves the stated problem while addressing all critical concerns from the initial review. The sibling resolution race condition is structurally eliminated, network access is properly gated, and the registry verification includes appropriate retry logic.

The canary validation mentioned in the PR description remains the only outstanding item, but the code changes themselves are sound and ready for merge pending that validation.

Job run: https://github.com/BitGo/BitGoJS/actions/runs/...
Branch: mihirseth/wcn-604-bitgo-shrinkwrap